Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make v2 default #50

Merged
merged 17 commits into from
Jul 24, 2023
Merged

Make v2 default #50

merged 17 commits into from
Jul 24, 2023

Conversation

rmccar
Copy link
Contributor

@rmccar rmccar commented Jul 19, 2023 •
edited
Loading

What is the context of this PR?

This is to change the version dropdown to default to v2. Just to make things a bit easier for the user.

  • Also fixes console error around a null field trying to be set to null
  • Tidies up some formatting
  • Refactors the show hide css js for some elements to make more sense
  • Fixes a bug with the default value for ref_p_start_date and ref_p_end_date not being used

How to review

  • Check that v2 is selected now when launcher is loaded
  • Check that schemas load as expected when selected from dropdown and using a url

@rmccar
Copy link
Contributor Author

rmccar commented Jul 19, 2023

Make v2 default on launcher

Yuyuutsu
Yuyuutsu reviewed Jul 19, 2023
View reviewed changes
Copy link

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally works , tested locally

katie-gardner
katie-gardner suggested changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all for having a default to v2 👍
Do we have any agreed formatting rules for this repo? 😆

templates/launch.html Outdated Show resolved Hide resolved
templates/launch.html Outdated Show resolved Hide resolved
katie-gardner
katie-gardner approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two tiny comments but other than that, looks good 👍

katie-gardner
katie-gardner approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved following the new changes 👍

MebinAbraham
MebinAbraham reviewed Jul 21, 2023
View reviewed changes
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting changes make this hard to review. It should have been separate. Plus formatting rules aren't enforced at the moment, so some else's IDE might be formatting differently. Happy for the formatting changes but it would have been nice if it was in a different PR.

@rmccar
Copy link
Contributor Author

rmccar commented Jul 21, 2023

The formatting changes make this hard to review. It should have been separate. Plus formatting rules aren't enforced at the moment, so some else's IDE might be formatting differently. Happy for the formatting changes but it would have been nice if it was in a different PR.

Yeah you're right. I'm happy to split them out into another PR. Ill do that 👍

Yuyuutsu
Yuyuutsu approved these changes Jul 24, 2023
View reviewed changes
Copy link

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@rmccar rmccar merged commit 39e0772 into main Jul 24, 2023
@rmccar rmccar deleted the default-version-to-v2 branch July 24, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MebinAbraham MebinAbraham MebinAbraham left review comments

@katie-gardner katie-gardner katie-gardner approved these changes

@Yuyuutsu Yuyuutsu Yuyuutsu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rmccar @MebinAbraham @Yuyuutsu @katie-gardner